CI: Overhaul static checks to use `pre-commit` #91597

Repiteo · 2024-05-05T18:23:35Z

Consolidates (almost) every single static_checks function/command into pre-commit directly. This has the benefit of adding these static checks as proper pre-commit hooks to the repo itself, so more issues have the potential to be caught well ahead of time. If possible, existing misc scripts were either replaced with a more parsable python equivalent, or removed entirely if a hook repo exists to achieve the same goal.

Only three exceptions remain that are still checked via static_checks: gitignore_check.sh, pytest, and xmllint. gitignore_check.sh is pretty specific to the workflow of that file specifically, so it's most sensible to keep as-is. xmllint technically has a hook available, but it requires docker & that's not nearly as likely for a user to have compared to npm/dotnet. pytest actually would've worked just fine, but based off a conversation² when I attempted to add it to pre-commit, it isn't a good candidate for a pre-commit so I'll leave it as-is.

akien-mga · 2024-05-08T08:08:02Z

Thanks for working on this! That's amazing.

I have yet to review the code properly, but some minor issues found when testing on Linux with pre-commit run --all:

file_format seems to find a bunch of stuff to fix that we missed in the master branch. I made Style: Trim trailing whitespace and ensure newline at EOF #91696 with those changes.
The eslint-html check errors out:

eslint-html..............................................................Failed
- hook id: eslint
- exit code: 2

Oops! Something went wrong! :(

ESLint: 8.46.0

ESLint couldn't find the config "airbnb-base" to extend from. Please check that the name of the config is correct.

The config "airbnb-base" was referenced from the config file in "/home/akien/Godot/godot/platform/web/.eslintrc.js".

If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.

Found by apply the file_format checks again via godotengine#91597.

akien-mga

Reviewed the code, looks really good.

akien-mga · 2024-05-08T14:15:25Z

.github/workflows/static_checks.yml

-      - name: Spell checks via codespell
-        if: github.event_name == 'pull_request' && env.CHANGED_FILES != ''
-        uses: codespell-project/actions-codespell@v2


Just noting that we'll miss one feature here, which is that this action provides a problem matcher in the GitHub diff view.

But in my experience most contributors don't seem to notice it (it's only visible in diff view, not on the PR itself), and the action was lacking in other configuration aspects so I think the tradeoffs are fine.

pre-commit should be set to output a git-diff log if any checks fail or changes are made, so it won't be that much of a loss.

.pre-commit-config.yaml

akien-mga · 2024-05-08T14:17:45Z

.pre-commit-config.yaml

+  - repo: https://github.com/pre-commit/mirrors-eslint
+    rev: v8.46.0
+    hooks:
+    - id: eslint


All these hooks have the same id, is that intended?

It is intentional, yes. They're all pulling from the same original hook implementing eslint, but it's called multiple times to replicate the multiple eslint checks we use. That's why name is different for each one, to differentiate the different checks during output.

akien-mga · 2024-05-08T14:31:09Z

BTW, if you want to have a look at how we could solve the problem those two PRs aimed to address (users not having python3 in PATH):

akien-mga · 2024-05-08T15:14:10Z

BTW, if you want to have a look at how we could solve the problem those two PRs aimed to address (users not having python3 in PATH):

Change pre-commit entry exe from python3 to python. #90634

Pre-commit: Specify default python version, rely on shebang #90662

I meant to tackle this possibly in a follow up PR, none of the two proposed solutions seem to work well cross-platform currently.

The current commit needs adding executable permission to files to work on Linux:

header-guards............................................................Failed
- hook id: header-guards
- exit code: 1

Executable `misc/scripts/header_guards.py` is not executable

file-format..............................................................Failed
- hook id: file-format
- exit code: 1

Executable `misc/scripts/file_format.py` is not executable

dotnet-format............................................................Failed
- hook id: dotnet-format
- exit code: 1

Executable `misc/scripts/dotnet_format.py` is not executable

But based on #90662 (review) this might not work on macOS.

Repiteo · 2024-05-08T15:43:46Z

I'll leave the python3 as-is for now then, and do some digging on how it's implemented in pre-commit. I think it can be setup such that it won't rely on shbang whatsoever, but I'm fine tackling that in a followup.

akien-mga · 2024-05-08T16:05:07Z

Beautiful:

clang-format.............................................................Passed
black....................................................................Passed
mypy.....................................................................Passed
codespell................................................................Passed
eslint-engine............................................................Passed
eslint-libs..............................................................Passed
eslint-sw................................................................Passed
eslint-html..............................................................Passed
make-rst.................................................................Passed
doc-status...............................................................Passed
jsdoc....................................................................Passed
copyright-headers........................................................Passed
header-guards............................................................Passed
file-format..............................................................Passed
dotnet-format............................................................Passed

akien-mga · 2024-05-08T16:33:18Z

Thanks! Great refactor 🎉

Found by apply the file_format checks again via godotengine#91597.

AThousandShips · 2024-05-11T13:03:23Z

It looks like this broke CI and no style checks are run at all, see:
https://github.com/godotengine/godot/actions/runs/9043846245/job/24851885660?pr=91829#step:6:82

Which has changed files but skips them

I suspect this is because it can't detect the files since there are no changed files

Repiteo · 2024-05-11T13:31:05Z

Then it probably needs that git diff logic passed to it after all, or something similar. Should be an easy fix either way, I'll get something up later today

patwork · 2024-05-12T15:20:33Z

@Repiteo Hi, would you be so kind as to take another look at .pre-commit-config.yaml? I've been working on upgrading eslint to version 9 and I see now that the automatic checks are returning errors:

https://github.com/godotengine/godot/pull/91863/checks

eslint-* because pre-commit is using eslint 8
jsdoc propably because some arguments are missing?

Is it possible for pre-commit to run npm run docs and npm run lint directly from /platform/web? Especially for eslint this is important, due to the fact that for version 9 you have to change the working directory first (the npm script does this).

Found by apply the file_format checks again via godotengine#91597.

Repiteo requested a review from a team as a code owner May 5, 2024 18:23

AThousandShips added enhancement topic:buildsystem labels May 6, 2024

AThousandShips added this to the 4.x milestone May 6, 2024

Repiteo force-pushed the ci/pre-commit-handle-everything branch 6 times, most recently from b626e7e to d9df5fc Compare May 7, 2024 15:27

akien-mga mentioned this pull request May 8, 2024

Style: Trim trailing whitespace and ensure newline at EOF #91696

Merged

akien-mga added a commit to akien-mga/godot that referenced this pull request May 8, 2024

Style: Trim trailing whitespace and ensure newline at EOF

7d03b1d

Found by apply the file_format checks again via godotengine#91597.

akien-mga approved these changes May 8, 2024

View reviewed changes

akien-mga mentioned this pull request May 8, 2024

Pre-commit: Move codespell script to pre-commit #88969

Closed

Repiteo force-pushed the ci/pre-commit-handle-everything branch from d9df5fc to 98052e2 Compare May 8, 2024 14:42

CI: Overhaul static checks to use pre-commit

df969ff

Repiteo force-pushed the ci/pre-commit-handle-everything branch from 98052e2 to df969ff Compare May 8, 2024 15:30

akien-mga approved these changes May 8, 2024

View reviewed changes

akien-mga modified the milestones: 4.x, 4.3 May 8, 2024

akien-mga merged commit 4778b24 into godotengine:master May 8, 2024
16 checks passed

Repiteo deleted the ci/pre-commit-handle-everything branch May 8, 2024 21:12

huwpascoe pushed a commit to huwpascoe/godot that referenced this pull request May 10, 2024

Style: Trim trailing whitespace and ensure newline at EOF

db7084b

Found by apply the file_format checks again via godotengine#91597.

AThousandShips mentioned this pull request May 11, 2024

[CI] Style checks are broken #91831

Closed

akien-mga mentioned this pull request May 11, 2024

CI: Re-add CHANGED_FILES logic for pre-commit checks #91835

Merged

patwork mentioned this pull request May 16, 2024

[CI] Fix for jsdoc hook in pre-commit #92013

Merged

dimitry- pushed a commit to AndroidWasm/godot that referenced this pull request May 16, 2024

Style: Trim trailing whitespace and ensure newline at EOF

54426a5

Found by apply the file_format checks again via godotengine#91597.

MewPurPur pushed a commit to MewPurPur/godot that referenced this pull request Jul 11, 2024

Style: Trim trailing whitespace and ensure newline at EOF

a848a90

Found by apply the file_format checks again via godotengine#91597.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: Overhaul static checks to use `pre-commit` #91597

CI: Overhaul static checks to use `pre-commit` #91597

Repiteo commented May 5, 2024 •

edited

Loading

akien-mga commented May 8, 2024

akien-mga left a comment

akien-mga May 8, 2024

Repiteo May 8, 2024

akien-mga May 8, 2024

Repiteo May 8, 2024

akien-mga commented May 8, 2024

akien-mga commented May 8, 2024

Repiteo commented May 8, 2024 •

edited

Loading

akien-mga commented May 8, 2024

akien-mga commented May 8, 2024

AThousandShips commented May 11, 2024 •

edited

Loading

Repiteo commented May 11, 2024

patwork commented May 12, 2024

CI: Overhaul static checks to use pre-commit #91597

CI: Overhaul static checks to use pre-commit #91597

Conversation

Repiteo commented May 5, 2024 • edited Loading

Footnotes

akien-mga commented May 8, 2024

akien-mga left a comment

Choose a reason for hiding this comment

akien-mga May 8, 2024

Choose a reason for hiding this comment

Repiteo May 8, 2024

Choose a reason for hiding this comment

akien-mga May 8, 2024

Choose a reason for hiding this comment

Repiteo May 8, 2024

Choose a reason for hiding this comment

akien-mga commented May 8, 2024

akien-mga commented May 8, 2024

Repiteo commented May 8, 2024 • edited Loading

akien-mga commented May 8, 2024

akien-mga commented May 8, 2024

AThousandShips commented May 11, 2024 • edited Loading

Repiteo commented May 11, 2024

patwork commented May 12, 2024

CI: Overhaul static checks to use `pre-commit` #91597

CI: Overhaul static checks to use `pre-commit` #91597

Repiteo commented May 5, 2024 •

edited

Loading

Repiteo commented May 8, 2024 •

edited

Loading

AThousandShips commented May 11, 2024 •

edited

Loading